Skip to content

Fix metrics for repartition#20924

Open
xanderbailey wants to merge 2 commits into
apache:mainfrom
xanderbailey:xb/fix-repartition-preserve-order-metrics-double-count
Open

Fix metrics for repartition#20924
xanderbailey wants to merge 2 commits into
apache:mainfrom
xanderbailey:xb/fix-repartition-preserve-order-metrics-double-count

Conversation

@xanderbailey

@xanderbailey xanderbailey commented Mar 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Found this when working on #20875

  • Closes #.

Rationale for this change

Metric reporting was previously incorrect for RepartitionExec if preserve order was set to true.

What changes are included in this PR?

Create new metrics before creating PerPartitionStream

Are these changes tested?

Yes and confirmed that this fails on main:

     thread 'repartition::test::test_preserve_order_output_rows_not_double_counted' (12487869)
      panicked at datafusion/physical-plan/src/repartition/mod.rs:3007:9:
     assertion `left == right` failed: metrics output_rows (8) should match actual rows
     collected (4), not double-count
       left: 8
      right: 4
     note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Are there any user-facing changes?

@xanderbailey xanderbailey force-pushed the xb/fix-repartition-preserve-order-metrics-double-count branch from 5a7328a to a5ccd80 Compare March 13, 2026 12:05
@xanderbailey xanderbailey mentioned this pull request Mar 13, 2026
Closed
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Mar 13, 2026
@xanderbailey xanderbailey force-pushed the xb/fix-repartition-preserve-order-metrics-double-count branch from a5ccd80 to 01598d9 Compare March 13, 2026 13:22
alamb
alamb reviewed Mar 13, 2026
View reviewed changes

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xanderbailey

Comment thread datafusion/physical-plan/src/repartition/mod.rs Outdated
spill_stream,
1, // Each receiver handles one input partition
BaselineMetrics::new(&metrics, partition),
BaselineMetrics::new(&intermediate_metrics, partition),

@alamb alamb Mar 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are just going to ignore the metrics, should we just remove them from PerPartitionStream ?

It seems like using a local copy of ExecutionPlanMetrics means they metrics in the PerPartitionStream are no longer accessable. So we can probably just remove the metrics to make it clearer they aren't used

@xanderbailey xanderbailey Mar 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good question, we still use PerPartitionStream here

datafusion/datafusion/physical-plan/src/repartition/mod.rs

Line 1089 in 01598d9

Ok(Box::pin(PerPartitionStream::new(
. We can make the metrics optional which is maybe more explicit?

@xanderbailey xanderbailey Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts here on what you'd prefer @alamb?

@xanderbailey xanderbailey Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it optional made the code inside PerPartitionStream less clean I think but I don't have a strong opinion I have to say so I'm happy to do whatever

@xanderbailey

xanderbailey commented Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

@alamb I made the metrics optional in 6ee4c22 let me know if you think that works?

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale PR has not had any activity for some time label Jun 28, 2026
@xanderbailey

xanderbailey commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Not stale please don’t close

@xanderbailey

xanderbailey commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

I’m away for a week or so and I’ll resolve the conflict then but I’d love to get the one merged if we can

@github-actions github-actions Bot removed the Stale PR has not had any activity for some time label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xanderbailey @alamb